feat: link package names to npm in html report - #1021
Conversation
sonukapoor
left a comment
There was a problem hiding this comment.
Good first PR - the approach is right and escapeHtml is used correctly throughout. Three small things to fix before we merge.
| : `<span class="fix-hint none" title="No known fix — consider replacing this package">⚠ No fix</span>`; | ||
|
|
||
| const depPathHtml = finding.dependencyPaths.length > 0 | ||
| const depPathHtml = finding.dependencyPaths.length > 0 |
There was a problem hiding this comment.
const depPathHtml lost its indent - should be 2 spaces in to match the surrounding code.
| return `<span class="dep-node${isLast ? " vulnerable" : ""}">${escapeHtml(node)}</span>${isLast ? "" : '<span class="dep-arrow">→</span>'}`; | ||
| const isFirst = i === 0; | ||
| const label = isFirst | ||
| ? escapeHtml(node) |
There was a problem hiding this comment.
The first hop is also a package on npm - no need to skip it. Remove the isFirst check and just always wrap node in the link.
| <td><span class="expand-icon" id="icon-${idx}">▶</span></td> | ||
| <td><div class="pkg-name">${escapeHtml(finding.package)}</div><div class="pkg-version">${escapeHtml(finding.version)}</div></td> | ||
| <td>${fixHtml}</td> | ||
| <td><div class="pkg-name"><a href="https://www.npmjs.com/package/${escapeHtml(finding.package)}" target="_blank" rel="noopener noreferrer">${escapeHtml(finding.package)}</a></div><div class="pkg-version">${escapeHtml(finding.version)}</div></td> <td>${fixHtml}</td> |
There was a problem hiding this comment.
<td>${fixHtml}</td> got merged onto the end of the line above. Put it on its own line.
sonukapoor
left a comment
There was a problem hiding this comment.
Good progress on removing the isFirst skip and using encodeURIComponent in the href. Two functional bugs to fix before merge.
| }).join("") | ||
| : `<span class="dep-node">${escapeHtml(finding.package)}</span>`; | ||
| && finding.dependencyPaths[0].map((node, i, arr) => { | ||
| const isLast = i === arr.length - 1; |
There was a problem hiding this comment.
Switching from a ternary to && dropped the fallback. When finding.dependencyPaths.length === 0, depPathHtml is now false, which renders as the literal text false in the HTML. Restore the ternary:
const depPathHtml = finding.dependencyPaths.length > 0
? finding.dependencyPaths[0].map((node, i, arr) => {
const isLast = i === arr.length - 1;
return `<a href="https://www.npmjs.com/package/${encodeURIComponent(node)}" target="_blank" rel="noopener noreferrer" class="dep-node${isLast ? " vulnerable" : "}">${escapeHtml(node)}</a>${isLast ? "" : '<span class="dep-arrow">→</span>'}`;
}).join("")
: `<span class="dep-node">${escapeHtml(finding.package)}</span>`;| const label = escapeHtml(node); | ||
|
|
||
| return `<a href="https://www.npmjs.com/package/${encodeURIComponent(node)}" target="_blank" rel="noopener noreferrer" class="dep-node${isLast ? " vulnerable" : ""}">${label}</a>${isLast ? "" : '<span class="dep-arrow">→</span>'}`;}) | ||
|
|
There was a problem hiding this comment.
Missing .join("") - .map() returns an Array, and when you interpolate it into the template string it joins with commas by default. Add .join("") after the closing }).
| <td><span class="expand-icon" id="icon-${idx}">▶</span></td> | ||
| <td><div class="pkg-name">${escapeHtml(finding.package)}</div><div class="pkg-version">${escapeHtml(finding.version)}</div></td> | ||
| <td>${fixHtml}</td> | ||
| <td><div class="pkg-name"><a href="https://www.npmjs.com/package/${escapeHtml(finding.package)}" target="_blank" rel="noopener noreferrer">${escapeHtml(finding.package)}</a></div><div class="pkg-version">${escapeHtml(finding.version)}</div></td> <td>${fixHtml}</td> |
There was a problem hiding this comment.
<td>${fixHtml}</td> is still on the same line as the pkg-name cell. Put it on its own line.
sonukapoor
left a comment
There was a problem hiding this comment.
Getting close - the ternary fallback is back, .join("") is there, and the dep-path links look right. Two more things to fix.
| import type { MaintenanceFinding } from "../maintenance/types.js"; | ||
| import { UNVERIFIED_PARENT_UPGRADE_NOTE, type SuggestedFixCommandPlan } from "../remediation/fix-commands.js"; | ||
| import { getCompletenessImpact } from "../scan/completeness.js"; | ||
| import { escape } from "node:querystring"; |
There was a problem hiding this comment.
This import is unused - escape from node:querystring is never called anywhere in the file. Remove it (it will also fail the build if noUnusedLocals is enabled).
| @@ -586,10 +587,9 @@ export function renderFindingRow(finding: SerializedFinding, idx: number, skippe | |||
| const rootDepsHtml = finding.rootDependencies.length > 0 | |||
| ? finding.rootDependencies.map(name => `<span class="root-dep">${escapeHtml(name)}</span>`).join(", ") | |||
There was a problem hiding this comment.
The pkg-name cell link uses escapeHtml() in the href, but it should use encodeURIComponent() - same as you're doing for dep path nodes above. escapeHtml converts & to & which breaks the URL. For scoped packages like @babel/core this would produce href="https://www.npmjs.com/package/@babel/core" correctly by accident, but the right function for URL encoding is encodeURIComponent:
<a href="https://www.npmjs.com/package/${encodeURIComponent(finding.package)}" ...>
sonukapoor
left a comment
There was a problem hiding this comment.
Both issues resolved - encodeURIComponent is used correctly throughout (dep path nodes, pkg-name cell, chain proof hops), and the unused escape import is gone. Merging.
|
hey,
do i need to merge this or it will be automatically merged as i cannot see
the option to merge
…On Sun, Aug 23, 2026 at 10:07 AM Sonu Kapoor ***@***.***> wrote:
***@***.**** approved this pull request.
Both issues resolved - encodeURIComponent is used correctly throughout
(dep path nodes, pkg-name cell, chain proof hops), and the unused escape
import is gone. Merging.
—
Reply to this email directly, view it on GitHub
<#1021?email_source=notifications&email_token=BXZDMU33Q272KLQ4WNM6MYT5LJYINA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTKMBQGE3DMMZYGE3KM4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#pullrequestreview-5001663816>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BXZDMU5EUB74TJQP6KEDO7T5LJYINAVCNFSNUABGKJSXA33TNF2G64TZHMYTCOJUGA2DMMRSG45US43TOVSTWNJSGA2DAMZTGYYDNILWAI>
.
Triage notifications, keep track of coding agent tasks and review pull
requests on the go with GitHub Mobile for iOS
<https://github.com/notifications/mobile/ios/BXZDMU34RCUMFINJA5N3WS35LJYINA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTKMBQGE3DMMZYGE3KM4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KUZTPN52GK4S7NFXXG>
and Android
<https://github.com/notifications/mobile/android/BXZDMU6XBLPNMHQXUWRTMVL5LJYINA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTKMBQGE3DMMZYGE3KM4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2K4ZTPN52GK4S7MFXGI4TPNFSA>.
Download it today!
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
sonukapoor
left a comment
There was a problem hiding this comment.
Nice addition - clickable npm links are a small but genuinely useful touch in the HTML report.
|
Merged - thank you @bharatmalik-cs! |
Fixes #1020
Wraps package names in dependency path chips, chain-proof hops, and finding header with links to their npm page (https://www.npmjs.com/package/).